Add Rehydrate, Enhancement for UI Tables and APIs - #2950
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
c2a8f79 to
4283a4b
Compare
asmasarw
changed the title
asmasarw
changed the title
921a331 to
57804d4
Compare
f224067 to
1450e57
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2950 +/- ##
=======================================
Coverage ? 32.85%
=======================================
Files ? 92
Lines ? 1960
Branches ? 471
=======================================
Hits ? 644
Misses ? 1265
Partials ? 51
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
mareklibra
left a comment
mareklibra
left a comment
There was a problem hiding this comment.
A few nitpicks.
The important comment is about error propagation.
| ), | ||
| width: '120px', | ||
| render: inst => { | ||
| const busy = rehydratingId === (inst.uid ?? ''); |
There was a problem hiding this comment.
Isn't following effectively the same?
| const busy = rehydratingId === (inst.uid ?? ''); | |
| const busy = rehydratingId === inst.uid; |
| ): Promise<CatalogItemInstance> { | ||
| return this.fetch<CatalogItemInstance>( | ||
| `catalog-item-instances/${catalogItemInstanceId}:rehydrate`, | ||
| { method: 'POST' }, |
There was a problem hiding this comment.
The Content-Type: application/json is set but body is missing.
Is it intentional? At least {} is a good practice.
There was a problem hiding this comment.
Yes, API is not expecting any body.
But It's a good approach to send an empty {} Object.
| resourcesApi | ||
| .listServiceTypeInstances() | ||
| .then(res => setInstances(res.instances ?? [])) | ||
| .catch(() => setInstances([])) |
There was a problem hiding this comment.
This silently swallows potential error.
It must be rendered for the user.
| const handleRehydrate = useCallback( | ||
| async (inst: CatalogItemInstance) => { | ||
| const id = inst.uid ?? ''; | ||
| if (!id) return; |
There was a problem hiding this comment.
I am not sure how the interface has been created.
Can you verify, that the uuid must be set as optional?
There was a problem hiding this comment.
The UUID is Optional because During Creation, we do not generate any ID for the Catalog item Instance.
UUID is generated after creating in server side (Backend).
| import { Box, Chip, IconButton, Tooltip, Typography } from '@material-ui/core'; | ||
| import { makeStyles } from '@material-ui/core/styles'; | ||
|
|
||
| const useStyles = makeStyles(() => ({ |
There was a problem hiding this comment.
Can you please move that behind the imports? It's a little but messy this way...
|
2 participants
Fixes Applied here: